chore(flushMicroTasks): Use the mocked scheduler when available #743

MatanBobi · 2020-07-12T19:46:27Z

Following a conversation with @eps1lon, he pointed out that maybe it would be better to mock the Scheduler (based on this PR by @acdlite).

This solution will handle both mocked version of scheduler and original version.

What: I've mocked the scheduler package in our setup-env and used the flushAll function which is available in the mocked scheduler package.

Why: React core team recommends to use a mocked version of the scheduler in test environments. We need to think if we want to recommend our users to mock the scheduler package also in their test environments. I think that if our users will mock the scheduler package, we won't need to know if someone is using fake timers or not and just call flushAll.

How: Checked for the presence of unstable_flushAll and used it if it exists.

Checklist:

Documentation added to the
docs site - N/A
Tests - N/A
Typescript definitions updated - N/A
Ready to be merged

I'm opening this one to create the conversation here and see what you think about it.

…ock the scheduler in our setup-env.

kentcdodds

I think we could simplify this a bit :)

kentcdodds · 2020-07-12T20:44:08Z

src/flush-microtasks.js

+      } else if (Scheduler && Scheduler.unstable_flushAll) {
+        Scheduler.unstable_flushAll()
+        enqueueTask(resolve)
      } else {
        scheduleCallback(() => enqueueTask(resolve))
      }


Suggested change

} else if (Scheduler && Scheduler.unstable_flushAll) {

Scheduler.unstable_flushAll()

enqueueTask(resolve)

} else {

scheduleCallback(() => enqueueTask(resolve))

}

} else {

Scheduler?.unstable_flushAll?.()

enqueueTask(resolve)

}

I think that in that case we won't scheduleCallback in case we're not using the mocked scheduler, right?

MatanBobi · 2020-07-13T05:57:17Z

Thanks @kentcdodds!
Do you have any thoughts on whether we should recommend our users to mock the scheduler package or not?

eps1lon · 2020-07-13T08:39:08Z

src/flush-microtasks.js

@@ -86,6 +87,9 @@ export default function flushMicroTasks() {
        // reproduce the problem, so there's no test for it. But it works!
        jest.advanceTimersByTime(0)
        resolve()
+      } else if (Scheduler && Scheduler.unstable_flushAll) {
+        Scheduler.unstable_flushAll()
+        enqueueTask(resolve)


We don't have to queue the task. Should be able resolve immediately.

I tried that, I wasn't able to understand why it didn't work and didn't get the time to look into it yesterday,
I thought it might be related to the fact that we're enqueuing a microtask in the destroy callback on the useEffect and flushAll doesn't wait for that.
I'll look into it today.

Are we supposed to wait for microtasks in cleanup functions? Does it even make sense to run async code in the cleanup function?

I'd we can avoid this being async that would be awesome, but I don't think we can avoid it because we can't assume people will be mocking the scheduler (especially when they're testing in environments with poor support for mocking).

I assumed that calling flushAll would mean we wouldn't need to worry about scheduling a callback. Sure wish we had a test that reproduces these timing issues. I have one in my bookshelf app that have me trouble before making all these changes. I can try these changes out there. I just couldn't simplify it to a reproducible example for inclusion in this project 😬

One suspicion I have is that you clean up your timers after the cleanup from testing-library runs. At least this is one issue I encountered in my mocha tests. Basically if you import from testing-libary we trigger an afterEach hook with cleanup and then in your test you call afterEach to cleanup timers. But this results in the cleanup of testing-libary being called with fake timers enabled which is fundamentally flawed (people can't expect to check for fake timers since they're not standardized).

If you can point me to the test in question I could check out if that is indeed the case.

😅 yeah, that's what I meant 👍👍

Thanks @eps1lon, I wasn't aware of the infinite loop that runAllTimers can cause. Using runOnlyPendingTimers sounds like a good approach but I'm not sure all testing frameworks have that ability (I think jasmine only has tick functionality but I've never really used it). Anyways, my examples will use jest.

Using runOnlyPendingTimers sounds like a good approach but I'm not sure all testing frameworks have that ability (I think jasmine only has tick functionality but I've never really used it). Anyways, my examples will use jest.

Which is why I don't think we should handle this in testing-library. Or at at least provide and entry point that makes no assumption about the testing framework.

Which is why I don't think we should handle this in testing-library. Or at at least provide and entry point that makes no assumption about the testing framework.

You're right.. Our plan wasn't to handle this within testing-library, just explain what should be done by our users in a new documentation page.. Do you think we shouldn't do that either?

Let's definitely do that 👍

kentcdodds · 2020-07-13T15:11:45Z

I think this is the right approach: #744

MatanBobi · 2020-07-13T17:57:04Z

I think this is the right approach: #744

Yes, definitely. I'm closing this one. Thanks!

kentcdodds · 2020-07-13T18:23:35Z

🎉 This issue has been resolved in version 10.4.6 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

codesandbox-ci · 2020-07-17T13:29:15Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6d56815:

Sandbox	Source
React	Configuration
kentcdodds/react-testing-library-examples	Configuration

Relates to testing-library/react-testing-library#743

chore(flushMicroTasks): Use the mocked scheduler when available and m…

6d56815

…ock the scheduler in our setup-env.

kentcdodds approved these changes Jul 12, 2020

View reviewed changes

eps1lon reviewed Jul 13, 2020

View reviewed changes

kentcdodds mentioned this pull request Jul 13, 2020

fix(cleanup): remove scheduler code from flush-microtasks #744

Merged

2 tasks

MatanBobi closed this Jul 13, 2020

kentcdodds added the released label Jul 13, 2020

MatanBobi added a commit to MatanBobi/testing-library-docs that referenced this pull request Jul 20, 2020

docs(fakeTimers): Explain how to use fake timers in testing-library

ab42bbd

Relates to testing-library/react-testing-library#743

MatanBobi mentioned this pull request Jul 21, 2020

docs(fakeTimers): Explain how to use fake timers in testing-library testing-library/testing-library-docs#528

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(flushMicroTasks): Use the mocked scheduler when available #743

chore(flushMicroTasks): Use the mocked scheduler when available #743

MatanBobi commented Jul 12, 2020 •

edited

Loading

kentcdodds left a comment

kentcdodds Jul 12, 2020

MatanBobi Jul 13, 2020

MatanBobi commented Jul 13, 2020

eps1lon Jul 13, 2020

MatanBobi Jul 13, 2020 •

edited

Loading

eps1lon Jul 13, 2020

kentcdodds Jul 13, 2020

eps1lon Jul 13, 2020

kentcdodds Jul 20, 2020

MatanBobi Jul 20, 2020

eps1lon Jul 20, 2020

MatanBobi Jul 20, 2020 •

edited

Loading

kentcdodds Jul 20, 2020

kentcdodds commented Jul 13, 2020

MatanBobi commented Jul 13, 2020

kentcdodds commented Jul 13, 2020

codesandbox-ci bot commented Jul 17, 2020

chore(flushMicroTasks): Use the mocked scheduler when available #743

chore(flushMicroTasks): Use the mocked scheduler when available #743

Conversation

MatanBobi commented Jul 12, 2020 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatanBobi commented Jul 13, 2020

Choose a reason for hiding this comment

MatanBobi Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatanBobi Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Jul 13, 2020

MatanBobi commented Jul 13, 2020

kentcdodds commented Jul 13, 2020

codesandbox-ci bot commented Jul 17, 2020

MatanBobi commented Jul 12, 2020 •

edited

Loading

MatanBobi Jul 13, 2020 •

edited

Loading

MatanBobi Jul 20, 2020 •

edited

Loading